Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve serialization perf and fix memory leak in SentryEvent #1693

Merged
merged 4 commits into from
Jun 7, 2022

Conversation

mattjohnsonpint
Copy link
Contributor

I ran into an issue wherein the SentryExceptions and SentryThreads properties on SentryEvent can return open JsonElement objects instead of their actual properties. If the JsonDocument holding those elements is not disposed, we get a memory leak. But also, if the document is disposed, we can't deserialize the data without getting an ObjectDisposedException. The root cause turned out to be an un-materialized IEnumerable on those properties coming from SentryEvent.FromJson.

The tests were hiding this, because we were using our Json.Parse(string) helper function, which was using .Clone(), so the elements wouldn't have been disposed with the document. Since cloning is semi-costly, I revised the helper methods to avoid it, by passing in a factory method delegate instead and using it to return a concrete object instead of a JsonElement.

Only one test failed with the change to the JSON helpers, which was for the issue had already found on SentryEvent. Adding .ToList() in the correct place made the test pass again.

So - part fix, part perf improvement.

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent! Thank you for finding and fixing this.

It could explain a an issue a user described on discord, without repro.

@mattjohnsonpint mattjohnsonpint merged commit 38b5c64 into main Jun 7, 2022
@mattjohnsonpint mattjohnsonpint deleted the fix-serialization branch June 7, 2022 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants